Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for profiling benchmarks using perf-record #214

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add support for profiling benchmarks using perf-record #214

wants to merge 3 commits into from

Conversation

mpage
Copy link

@mpage mpage commented Jan 27, 2025 •
edited
Loading

This PR adds support for profiling benchmarks using a perf-record hook. perf-record is enabled only during benchmark execution by using perf's support for control fifos.

For example, to collect stacks using DWARF debug information, one could run:

env PYPERF_PERF_RECORD_EXTRA_OPTS="--call-graph dwarf -e cycles:pp" \
       python -m pyperf timeit \
                    --debug-single-value \
                    --hook perf_record \
                    --inherit-environ PYPERF_PERF_RECORD_EXTRA_OPTS \
                    '1+1'

This should resolve #213.

@corona10 corona10 requested review from mdboom and corona10 February 5, 2025 17:56
@corona10
Copy link
Member

corona10 commented Feb 5, 2025

I will try to take a look at it by this weekend :)

@mdboom mdboom mentioned this pull request Feb 5, 2025
Open
corona10
corona10 reviewed Feb 5, 2025
View reviewed changes
pyperf/_hooks.py
def exec_perf_cmd(self, cmd):
self.ctl_fd.write(f"{cmd}\n")
self.ctl_fd.flush()
res = self.ack_fd.readline()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
res = self.ack_fd.readline()
_ = self.ack_fd.readline()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for passing the CI

corona10
corona10 reviewed Feb 5, 2025
View reviewed changes
pyperf/_hooks.py
appended to the command line of perf-record, if provided.
"""

def mkfifo(self, tmpdir, basename):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we move down mkfifo / exec_perf_cmd to under the dunder methods?

mdboom
mdboom approved these changes Feb 5, 2025
View reviewed changes
Copy link
Collaborator

@mdboom mdboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. This is a tricky one to add a test and CI for -- I did test it locally and it "worked for me". I think for most purposes we can rely on bench_runner to ensure this doesn't bit rot.

@corona10
Copy link
Member

corona10 commented Feb 5, 2025

Would you like to update the documentation for it? https://github.com/psf/pyperf/blob/f3131745472b108b95cd5e20733cdf4a20ec0a37/doc/run_benchmark.rst#specializer-statistics-pystats

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corona10 corona10 corona10 left review comments

@mdboom mdboom mdboom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support profiling benchmarks using perf-record
3 participants
@mpage @corona10 @mdboom